feat(katana): add builder pattern for katana config #2653
Conversation
WalkthroughOhayo, sensei! This pull request introduces significant changes to the configuration management in the Katana project. The main focus is the introduction of a Changes
Assessment against linked issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
crates/dojo/test-utils/src/sequencer.rs (1)
8-9: Consider being more specific with imports.Ohayo! While the ApiKind import is nicely specific, the wildcard import
katana_node::config::*might expose more symbols than needed. Consider explicitly importing justConfigBuilderand other required types.-pub use katana_node::config::*; +use katana_node::config::{Config, ConfigBuilder};bin/katana/src/cli/node.rs (1)
285-295: Excellent implementation of the builder pattern, sensei!The implementation follows best practices by:
- Breaking down configuration into logical components
- Making the configuration process more intuitive
- Maintaining a clear separation of concerns
This change significantly improves maintainability and makes the configuration process more flexible for users.
Consider adding validation methods in the builder to ensure configuration consistency before the final build step. This could prevent invalid combinations of settings early in the configuration process.
crates/katana/node/src/config/mod.rs (2)
216-245: Ohayo, sensei! Avoid redundant assignments and cloning inmessaging_*methodsIn the
messaging_*methods, the fields are being assigned twice, and cloning of parameters is unnecessary. You can simplify the code by initializing withDefault::default()and setting the fields afterward, avoiding redundant assignments and cloning.For example, update the
messaging_chainmethod:pub fn messaging_chain(mut self, chain: String) -> Self { self.config - .messaging - .get_or_insert(MessagingConfig { chain, ..Default::default() }) - .chain = chain.clone(); + .messaging + .get_or_insert_with(Default::default) + .chain = chain; self }Apply similar changes to other
messaging_*methods.
163-169: Ohayo, sensei! Simplifyfork_blockmethod initialization withDefault::default()In the
fork_blockmethod, initializingForkingConfigwith an empty URL usingUrl::from_str("").unwrap()is less ideal. Consider usingDefault::default()for cleaner initialization.Apply this diff:
pub fn fork_block(mut self, block: Option<BlockHashOrNumber>) -> Self { self.config .forking - .get_or_insert(ForkingConfig { url: Url::from_str("").unwrap(), block: None }) + .get_or_insert_with(Default::default) .block = block; self }This improves code clarity and avoids potential issues with invalid URLs.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
bin/katana/src/cli/node.rs(5 hunks)crates/dojo/test-utils/src/sequencer.rs(2 hunks)crates/katana/core/Cargo.toml(0 hunks)crates/katana/core/src/constants.rs(1 hunks)crates/katana/node/src/config/execution.rs(1 hunks)crates/katana/node/src/config/mod.rs(2 hunks)
💤 Files with no reviewable changes (1)
- crates/katana/core/Cargo.toml
✅ Files skipped from review due to trivial changes (1)
- crates/katana/node/src/config/execution.rs
🔇 Additional comments (7)
crates/katana/core/src/constants.rs (2)
12-12: Ohayo! Nice optimization using const instead of lazy_static!
This change improves performance by moving the initialization to compile-time and reduces runtime overhead. The code is also more maintainable without the lazy_static machinery.
12-12: Verify ContractAddress const-compatibility
Let's verify that ContractAddress is suitable for const initialization and that all its components support compile-time construction.
crates/dojo/test-utils/src/sequencer.rs (2)
114-120: Implementation aligns well with PR objectives.
Ohayo sensei! The builder pattern implementation effectively addresses the goals from issue #2648:
- ✅ Simplifies configuration setup
- ✅ Allows individual value configuration
- ✅ Clean and maintainable code structure
The method chaining approach is particularly elegant and readable.
114-120:
Function signature inconsistency with behavior.
Ohayo sensei! The implementation looks clean, but there's a discrepancy between the function name/signature and its behavior:
- The function name
get_default_test_configsuggests it returns aConfig, but it returns a built configuration - The AI summary indicates this should return
Config, notConfigBuilder
Consider this fix:
-pub fn get_default_test_config(sequencing: SequencingConfig) -> Config {
- ConfigBuilder::new()
- .dev_fee(false)
- .chain_id(ChainId::SEPOLIA)
- .genesis_sequencer_address(DEFAULT_SEQUENCER_ADDRESS)
- .rpc_apis(HashSet::from([ApiKind::Starknet, ApiKind::Dev, ApiKind::Saya, ApiKind::Torii]))
- .sequencing(sequencing)
- .build()
+pub fn get_default_test_config(sequencing: SequencingConfig) -> Config {
+ ConfigBuilder::new()
+ .dev_fee(false)
+ .chain_id(ChainId::SEPOLIA)
+ .genesis_sequencer_address(DEFAULT_SEQUENCER_ADDRESS)
+ .rpc_apis(HashSet::from([ApiKind::Starknet, ApiKind::Dev, ApiKind::Saya, ApiKind::Torii]))
+ .sequencing(sequencing)
+ .build()
+ .expect("Failed to build default test config")
}Let's verify the usage of this function across the codebase:
bin/katana/src/cli/node.rs (3)
34-34: Ohayo sensei! Import change looks good!
The import change aligns perfectly with the introduction of the builder pattern.
328-328: Clean and direct constant usage, sensei!
The direct usage of DEFAULT_SEQUENCER_ADDRESS simplifies the code and improves readability.
536-536: Test assertions properly updated, sensei!
The test cases have been correctly updated to verify the new configuration behavior with DEFAULT_SEQUENCER_ADDRESS.
Also applies to: 562-562
crates/katana/node/src/config/mod.rs
Outdated
| self.config | ||
| .messaging | ||
| .get_or_insert(MessagingConfig { sender_address, ..Default::default() }) | ||
| .sender_address = sender_address.clone(); | ||
| self | ||
| } | ||
|
|
||
| pub fn messaging_private_key(mut self, private_key: String) -> Self { | ||
| self.config | ||
| .messaging | ||
| .get_or_insert(MessagingConfig { private_key, ..Default::default() }) | ||
| .private_key = private_key.clone(); | ||
| self | ||
| } | ||
|
|
||
| pub fn messaging_interval(mut self, interval: u64) -> Self { | ||
| self.config | ||
| .messaging | ||
| .get_or_insert(MessagingConfig { interval, ..Default::default() }) | ||
| .interval = interval; | ||
| self | ||
| } | ||
|
|
||
| pub fn messaging_from_block(mut self, from_block: u64) -> Self { | ||
| self.config | ||
| .messaging | ||
| .get_or_insert(MessagingConfig { from_block, ..Default::default() }) | ||
| .from_block = from_block; | ||
| self | ||
| } | ||
|
|
||
| pub fn sequencing_block_time(mut self, block_time: Option<u64>) -> Self { | ||
| self.config.sequencing.block_time = block_time; | ||
| self | ||
| } | ||
|
|
||
| pub fn sequencing_no_mining(mut self, no_mining: bool) -> Self { | ||
| self.config.sequencing.no_mining = no_mining; | ||
| self | ||
| } | ||
|
|
||
| pub fn dev_fee(mut self, fee: bool) -> Self { | ||
| self.config.dev.fee = fee; | ||
| self | ||
| } | ||
|
|
||
| pub fn dev_account_validation(mut self, validation: bool) -> Self { | ||
| self.config.dev.account_validation = validation; | ||
| self | ||
| } | ||
|
|
||
| pub fn dev_fixed_gas_prices(mut self, gas_prices: Option<FixedL1GasPriceConfig>) -> Self { | ||
| self.config.dev.fixed_gas_prices = gas_prices; | ||
| self | ||
| } | ||
|
|
||
| pub fn chain(mut self, chain: ChainSpec) -> Self { | ||
| self.config.chain = chain; | ||
| self | ||
| } | ||
|
|
||
| pub fn db(mut self, db: DbConfig) -> Self { | ||
| self.config.db = db; | ||
| self | ||
| } | ||
|
|
||
| pub fn rpc(mut self, rpc: RpcConfig) -> Self { | ||
| self.config.rpc = rpc; | ||
| self | ||
| } | ||
|
|
||
| pub fn metrics(mut self, metrics: Option<MetricsConfig>) -> Self { | ||
| self.config.metrics = metrics; | ||
| self | ||
| } | ||
|
|
||
| pub fn execution(mut self, execution: ExecutionConfig) -> Self { | ||
| self.config.execution = execution; | ||
| self | ||
| } | ||
|
|
||
| pub fn messaging(mut self, messaging: Option<MessagingConfig>) -> Self { | ||
| self.config.messaging = messaging; | ||
| self | ||
| } | ||
|
|
||
| pub fn sequencing(mut self, sequencing: SequencingConfig) -> Self { | ||
| self.config.sequencing = sequencing; | ||
| self | ||
| } | ||
|
|
||
| pub fn dev(mut self, dev: DevConfig) -> Self { | ||
| self.config.dev = dev; | ||
| self | ||
| } | ||
|
|
||
| pub fn build(self) -> Config { | ||
| self.config | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Consider changing method signatures to use &mut self and return &mut Self for idiomatic builder pattern
In Rust, it's idiomatic for builder pattern methods to take &mut self and return &mut Self. This allows for method chaining without consuming self at each step, improving efficiency by avoiding unnecessary copies.
Apply this diff to update the method signatures:
- pub fn chain_id(mut self, chain_id: ChainId) -> Self {
+ pub fn chain_id(&mut self, chain_id: ChainId) -> &mut Self {
self.config.chain.id = chain_id;
self
}
- pub fn genesis_parent_hash(mut self, parent_hash: BlockHash) -> Self {
+ pub fn genesis_parent_hash(&mut self, parent_hash: BlockHash) -> &mut Self {
self.config.chain.genesis.parent_hash = parent_hash;
self
}
// ... Apply similar changes to all other builder methodsCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
crates/katana/node/src/config/mod.rs (3)
75-78: Add documentation for ConfigBuilder struct.Ohayo, sensei! Consider adding documentation comments to explain the purpose and usage of the ConfigBuilder struct, similar to how the Config struct is documented.
Add this documentation:
+/// Builder for creating node configurations. +/// +/// Provides a fluent interface for constructing a `Config` instance, +/// allowing users to set only the values they want to customize +/// while keeping defaults for others. #[derive(Default, Debug)] pub struct ConfigBuilder { config: Config, }
216-270: Optimize messaging methods by removing unnecessary clones.The messaging methods contain unnecessary clone() calls since the values are already being moved into the config.
Here's an example fix for the messaging_chain method (apply similar changes to other messaging methods):
pub fn messaging_chain(&mut self, chain: String) -> &mut Self { self.config .messaging - .get_or_insert(MessagingConfig { chain, ..Default::default() }) - .chain = chain.clone(); + .get_or_insert(MessagingConfig { chain: chain.clone(), ..Default::default() }) + .chain = chain; self }
337-339: Consider adding validation in build method.The current implementation doesn't validate if required configuration fields are set before building the final config.
Consider adding a validate() method that checks for required fields and valid combinations of settings. For example:
- Ensure chain ID is set
- Validate that fork block is not set without fork URL
- Verify that messaging configuration has all required fields when enabled
bin/katana/src/cli/node.rs (1)
329-329: Consider adding a comment explaining the sequencer address assignment.While the code is correct, it would be helpful to add a brief comment explaining why we're setting the default sequencer address here, especially since this is part of the genesis configuration.
+ // Set the default sequencer address if no custom genesis is provided chain_spec.genesis.sequencer_address = DEFAULT_SEQUENCER_ADDRESS;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
bin/katana/src/cli/node.rs(5 hunks)crates/katana/node/src/config/mod.rs(2 hunks)
🔇 Additional comments (3)
bin/katana/src/cli/node.rs (3)
34-34: LGTM! Clean import update.
The import change from Config to ConfigBuilder aligns with the PR objective of implementing the builder pattern.
285-296: Excellent implementation of the builder pattern, sensei!
The implementation follows the builder pattern best practices:
- Clear method chaining
- Logical grouping of configuration components
- Final
build()call to construct the config
The code is more readable and maintainable compared to direct struct initialization.
537-537: LGTM! Test assertions properly updated.
The test assertions have been correctly updated to verify the sequencer address configuration in both default and custom scenarios.
Also applies to: 563-563
| pub fn build(&mut self) -> Config { | ||
| self.config.clone() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize build method to avoid unnecessary cloning.
The build method clones the entire config unnecessarily. Since this is the final step in the builder pattern, we can take ownership of the config.
Apply this change:
-pub fn build(&mut self) -> Config {
- self.config.clone()
+pub fn build(self) -> Config {
+ self.config
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn build(&mut self) -> Config { | |
| self.config.clone() | |
| } | |
| pub fn build(self) -> Config { | |
| self.config | |
| } |
| pub fn fork_block(&mut self, block: Option<BlockHashOrNumber>) -> &mut Self { | ||
| self.config | ||
| .forking | ||
| .get_or_insert(ForkingConfig { url: Url::from_str("").unwrap(), block: None }) | ||
| .block = block; | ||
| self | ||
| } |
There was a problem hiding this comment.
Improve fork_block implementation.
The empty URL string used as default in fork_block is not a meaningful default and could cause issues.
Consider requiring the URL to be set before the block can be set:
-pub fn fork_block(&mut self, block: Option<BlockHashOrNumber>) -> &mut Self {
- self.config
- .forking
- .get_or_insert(ForkingConfig { url: Url::from_str("").unwrap(), block: None })
- .block = block;
- self
+pub fn fork_block(&mut self, block: Option<BlockHashOrNumber>) -> &mut Self {
+ if let Some(forking) = &mut self.config.forking {
+ forking.block = block;
+ } else {
+ panic!("Fork URL must be set before setting fork block");
+ }
+ self
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn fork_block(&mut self, block: Option<BlockHashOrNumber>) -> &mut Self { | |
| self.config | |
| .forking | |
| .get_or_insert(ForkingConfig { url: Url::from_str("").unwrap(), block: None }) | |
| .block = block; | |
| self | |
| } | |
| pub fn fork_block(&mut self, block: Option<BlockHashOrNumber>) -> &mut Self { | |
| if let Some(forking) = &mut self.config.forking { | |
| forking.block = block; | |
| } else { | |
| panic!("Fork URL must be set before setting fork block"); | |
| } | |
| self | |
| } |
There was a problem hiding this comment.
Thank you @edisontim for the work here!
@kariy that's a lot of functions and things to adjust if one of the config type changes, but the code simplification is actually pretty great, wdyt?
I feel like having a builder for each config may be a better approach, keeping the high level function @edisontim did?
let rpc = RpcConfigBuilder::new()
.port(0)
.addr(DEFAULT_RPC)
.max_connections(DEFAULT_MAXCONN);
let config = ConfigBuilder::new()
.rpc(rpc.build())
.dev(DevConfigBuild::new().fee(false))
.build();
|
@edisontim you would have to rebase due to latest changes. 👍 |
glihm
changed the title
I agree with the second point, I think it's a lot of clutter in one file. However for the first point it's the same if you split it in different configs no? The advantage of keeping it as one struct though as @kariy raised is that you only need one import for your config, in the case you split it you'll need to import multiple builders Not sure abt this one |
I see, let's wait @kariy input here to be sure we're all aligned then. :) |
|
@edisontim this may require a little rebase if you have a chance before @kariy review. 🫡 |
Description
Adds a
ConfigBuilderbuilder pattern to theConfigstruct of KatanaRelated issue
Closes #2648
Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
Release Notes
New Features
ConfigBuilderfor more modular and readable configuration setup.Bug Fixes
DEFAULT_SEQUENCER_ADDRESSfor improved clarity.Refactor
lazy_staticto direct constant declarations forDEFAULT_SEQUENCER_ADDRESS.ExecutionConfigfor better structure.NodeArgsand test sequencer.Chores